[SPARK-57891][CORE] Add CredentialProvider SPI and ServiceLoader discovery#57191
[SPARK-57891][CORE] Add CredentialProvider SPI and ServiceLoader discovery#57191yadavay-amzn wants to merge 3 commits into
Conversation
…or single candidate; clarify init-once-per-instance
| // Initialize exactly once under the lock (first-conf-wins). | ||
| synchronized (CredentialProviderLoader.class) { | ||
| if (!initializedProviders.contains(selected)) { | ||
| selected.init(conf); |
There was a problem hiding this comment.
I have a security concern here. The full Spark configuration map is passed to init(). Since providers are discovered via ServiceLoader (potentially third-party JARs), a buggy or malicious provider receives all config properties including secrets from other subsystems (e.g., spark.authenticate.secret, database passwords, cloud keys).
Spark has precedent for namespace scoping. DataSourceV2Utils.extractSessionConfigs() filters the full conf to only spark.datasource.<keyPrefix>.* before passing to providers. Similar to this approach, can we filter the map to only pass spark.security.credentials.* keys?
| * @throws IllegalStateException if a provider returns null from {@code supportedSchemes()} | ||
| */ | ||
| public static Optional<CredentialProvider> providerFor(String scheme, Map<String, String> conf) { | ||
| Objects.requireNonNull(scheme, "scheme must not be null"); |
There was a problem hiding this comment.
Null scheme is properly rejected, but an empty string "" passes through and silently returns Optional.empty(). An empty scheme is never valid per URI semantics (RFC 3986) and is likely always a caller bug. Can we add the following check?
if (scheme.isEmpty()) {
throw new IllegalArgumentException("scheme must not be empty");
}| * @throws CredentialResolutionException if the credential exchange fails | ||
| * @since 4.3.0 | ||
| */ | ||
| ServiceCredential resolve(UserContext user, URI target) throws CredentialResolutionException; |
There was a problem hiding this comment.
Since CredentialProviderLoader caches and reuses provider instances (singleton), resolve() may be called from multiple threads (e.g., credential refresh scheduling). Can we document whether implementations must be thread-safe ? (interface-level or resolve() Javadoc)
Suggestion for interface-level Javadoc:
Implementations must be thread-safe: {@code resolve()} may be called concurrently
from multiple threads after {@code init()} completes.
| // Initialize exactly once under the lock (first-conf-wins). | ||
| synchronized (CredentialProviderLoader.class) { | ||
| if (!initializedProviders.contains(selected)) { | ||
| selected.init(conf); |
There was a problem hiding this comment.
What happens if init() throws? Currently the provider is not added to initializedProviders, so a subsequent call would retry init(). This retry behavior seems reasonable, but:
- The provider instance may be in a half-initialized state after a failed
init(). Is that safe to retry? - Should the contract document whether
init()is expected to be idempotent-safe (i.e., safe to call again after a previous failure)?
How about:
- Documenting the retry-on-failure behavior in the
init()Javadoc (e.g., "If init() throws, it may be retried on the next resolution attempt. Implementations should be safe to call again after a prior failure."), or - Adding a test verifying this behavior
|
Let's remove the "Design notes (feedback/suggestions welcome)" section from the description because it's used as commit log when this PR is merged. |
2 participants
What changes were proposed in this pull request?
This is task 2 of the OIDC Credential Propagation SPIP (SPARK-57703), building on the core types added in SPARK-57890. It introduces the pluggable provider SPI in
org.apache.spark.security(spark-core):CredentialProvider- a@DeveloperApiinterface that providers (AWS STS, Azure, GCP, etc.) implement to exchange a user identity for a short-livedServiceCredentialCredentialResolutionException- a@DeveloperApichecked exception thrown byresolve.CredentialProviderLoader- discovers implementations viaServiceLoaderand selects a provider per scheme. Scheme keys are normalized to lowercase. Selection follows an explicit-configuration policy:spark.security.credentials.provider.<scheme>names the fully-qualified provider class to use for that scheme. If exactly one discovered provider supports a scheme, no configuration is needed; if multiple support it and the conf is unset, a clear error is raised listing the candidates. Each selected provider is initialized exactly once.A
FakeCredentialProvider(and a second one sharing a scheme) plus aMETA-INF/servicesregistration are added undercore/src/testto exercise discovery and selection.Why are the changes needed?
There is currently no pluggable SPI for exchanging an identity token for short-lived service credentials. This is the extension point the credential-propagation framework builds on. See the SPIP design document, Appendix A.
Does this PR introduce any user-facing change?
No behavior change. It adds new
@DeveloperApitypes only; nothing uses them until the follow-up (SPARK-57892 / the manager task).How was this patch tested?
New JUnit
CredentialProviderLoaderSuitecovering the acceptance criteria, ServiceLoader discovery, provider selection/ambiguity, error cases, and null-input guards.Design notes (feedback/suggestions welcome)
Two choices worth considering:
spark.security.credentials.provider.<scheme>(single fully-qualified class name). This can be extended to an ordered-priority list later without breaking the conf if preferred.initlifecycle: each provider is initialized once, on first selection, with that call's configuration (first-conf-wins), inside the loader's lock. This sets the provider lifecycle the manager task will consume.Was this patch authored or co-authored using generative AI tooling?
No.